Skip to content

adding a password reset system for admins#246

Open
vlad-a-c wants to merge 5 commits into
XGDevGroup:mainfrom
vlad-a-c:main
Open

adding a password reset system for admins#246
vlad-a-c wants to merge 5 commits into
XGDevGroup:mainfrom
vlad-a-c:main

Conversation

@vlad-a-c
Copy link
Copy Markdown
Contributor

since we don't have a mandatory email address entered, the only way of changing a password is for an admin to create a temp pass. i'm also thinking of making the user able to change his own password from the options menu.

@zarvox32
Copy link
Copy Markdown
Contributor

zarvox32 commented May 1, 2026

Reviewed and tested locally on the merge ref. All 49 tests in the three modified test files pass, plus 43 tests in test_server_authorization.py, test_server_message_flow.py, and test_server_core_misc.py — no regressions in the auth/menu surface this PR touches.

The auth changes are the most valuable part: reset_password now invalidates in-memory sessions AND revokes refresh tokens at the DB layer, which closes a real gap where a reset password wouldn't actually log the target out everywhere. The new revoke_user_refresh_tokens correctly uses lower(username) = lower(?) matching the rest of the table.

A few non-blocking observations (take or leave):

nit: getattr(self, "_password_min_length", 8) and the matching _password_max_length default in _handle_reset_password_editboxServer.__init__ always sets these, so the fallback is dead code. Registration in core/server.py uses self._password_min_length directly. Minor consistency thing.

nit: reset-user-password-prompt doesn't include the "or press Escape to cancel" hint that decline-reason-prompt has. An admin who selects the wrong user and wants to back out has no clear way to discover that.

future consideration: The editbox protocol has no masked/password input mode, so the temporary password is likely echoed and spoken by the screen reader as the admin types it. Pre-existing limitation, not introduced here, but worth filing as an issue if we ever add a self-service password change flow — it'd be the same problem at a worse blast radius.

future consideration: After a successful reset the admin lands back on the user list, which matches _ban_user's pattern so it's consistent — but for password reset specifically, returning to the admin menu might be more useful since the typical flow is "reset one user, done." Not worth changing in isolation.

The double trust-level check (decorator + explicit target_record.trust_level.value >= TrustLevel.ADMIN.value guard inside _reset_user_password) is good defense in depth.

LGTM.

@zarvox32 zarvox32 linked an issue May 1, 2026 that may be closed by this pull request
@zarvox32
Copy link
Copy Markdown
Contributor

Re-tested the merge ref now that 05ad0f1 Fix mac app sound resource paths has landed on top of the password-reset commits.

Scope concern. The PR title and description only cover admin password reset, but 05ad0f1 bundles in 84 LOC of unrelated macOS PyInstaller work (PlayPalace.spec, pyinstaller_runtime.py, sound_manager.py, a sound-manager test). That commit landed after my earlier review of the password-reset feature, so it hasn't actually been reviewed here. Splitting it into its own PR would keep history readable and let the password-reset feature merge on its own merits.

Test results.

Server side (the password-reset feature) is clean — test_administration.py, test_database.py, test_integration.py: 49/49 pass on the merge ref. Nothing in 79061d9 Address password reset review feedback regressed.

Desktop side has a real bug introduced by 05ad0f1:

clients/desktop/tests/test_sound_manager.py::test_default_sounds_folder_is_absolute FAILED

E       AssertionError: assert False
E        +  where False = 'C:\Users\me\Documents\PlayPalace11\clients\desktop\sounds'.startswith('/')

Line 48 checks absoluteness with manager.sounds_folder.startswith(\"/\"), which is POSIX-only — Windows absolute paths start with a drive letter. The fix is one line: replace it with os.path.isabs(manager.sounds_folder), which is also a more accurate assertion of what's actually being claimed.

CI. The claude-review failure is infrastructure — OIDC token exchange returned 401 on all three retries (Invalid OIDC token). Not a code issue.

Other notes on the mac commit (non-blocking).

  • pyinstaller_runtime.py does os.chdir(os.path.dirname(sys.executable)) at runtime-hook load when frozen. That mutates cwd for the whole process — fine for a one-shot launch, but anything else in the client using relative paths inherits it. Worth being aware of even if nothing breaks today.
  • _default_sounds_folder() itself is correct in both source and frozen modes; the production logic is fine, only the test is broken.
  • The spec file is mac-only (.app / Contents/MacOS/sounds). The project's build story now has a Windows/Linux gap that someone will need to fill before this can ship to non-mac users.

Recommendation. Two clean paths:

  1. Split the mac fix into a separate PR. Cleanest history, lets the password-reset feature land today.
  2. Keep it bundled, but update the PR title/description to reflect both changes and fix the Windows-incompatible test before merge.

Either way, the startswith(\"/\") assertion needs to go. The password-reset feature itself I'm still happy with — the auth-layer gap-closing (session invalidation + refresh-token revocation on reset) is the most valuable part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Add password retrieval system.

2 participants